-
Notifications
You must be signed in to change notification settings - Fork 0
driver: implement tkv driver for tarantool support #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Introduce a new Tarantool TKV driver and align core interfaces and models to support it
- Add TKV driver with transactional execution and key watch support
- Adjust driver interfaces (Watch signature) and data models (KeyValue, RequestResponse) for compatibility
- Add integration tests against a Tarantool instance and update dependencies and lint configuration
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| driver/tkv/tkv.go | Implements TKV driver Execute and Watch, wiring to Tarantool watcher API. |
| driver/tkv/txn.go | Defines request/response encoding/decoding and maps Tarantool responses to tx.Response. |
| driver/tkv/operations.go | Encodes operations (get/put/delete) for TKV. |
| driver/tkv/predicate.go | Encodes predicates and maps ops/targets to TKV equivalents. |
| driver/tkv/integration_test.go | Integration tests validating put/get/delete, predicates, multi-op, and watch behavior. |
| driver/driver.go | Changes Watch API to return a channel, a stop function, and an error. |
| driver/etcd/etcd.go | Updates Watch signature to match the new Driver interface. |
| watch/event.go | Simplifies Event to contain only Prefix. |
| tx/requestresponse.go | Refactors RequestResponse to hold a slice of KeyValue values. |
| kv/kv.go | Removes CreateRevision and Version fields; keeps ModRevision. |
| internal/mocks/driver_mock.go | Updates mocks to match the new Watch signature. |
| go.mod | Adds msgpack/v5 as a direct dependency. |
| .golangci.yml | Allows msgpack imports and tarantool packages in tests. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
7531a92 to
c5c3cb7
Compare
Pull Request Test Coverage Report for Build 18777587711Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
driver/tcs/tcs.go
Outdated
| return rvChan, func() { close(isStopped) }, nil | ||
| } |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The returned stop function will panic if called more than once due to closing the same channel twice. Make it idempotent, e.g., by wrapping the close with a sync.Once or an atomic guard.
Copilot uses AI. Check for mistakes.
870c579 to
2df1217
Compare
2df1217 to
2c88b0b
Compare
2c88b0b to
1d5d4ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, rename TKV -> TcS or TCS everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, update the CI impl commit title & message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a nice idea to start the CHANGELOG.md here. Or you could it fill on the first release. Up to you.
|
|
||
| env: | ||
| # Note: Use exactly match version of tool, to avoid unexpected issues with test on CI. | ||
| GO_VERSION: '1.23.8' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the library requires 1.24 in the go.mod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| - name: Setup python | ||
| uses: actions/setup-python@v4 | ||
| with: | ||
| python-version: '${{ env.PYTHON_VERSION }}' | ||
|
|
||
| - name: Setup Mage | ||
| run: | | ||
| git clone https://github.com/magefile/mage | ||
| cd mage | ||
| go run bootstrap.go | ||
| shell: bash | ||
|
|
||
| - name: Install build requirements | ||
| run: | | ||
| sudo apt -y update | ||
| sudo apt -y install git gcc make cmake unzip zip fish zsh | ||
| shell: bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the stuff here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| tar -xzf ${ARCHIVE_NAME} | ||
| rm -f ${ARCHIVE_NAME} | ||
| source tarantool-enterprise/env.sh | ||
| shell: bash No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| - name: Setup Go | ||
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: '${{ env.GO_VERSION }}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably we don't need the Go here. It is better to install the Go in a job pipeline directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| ${INSTALL_PREFIX}/etcd --version | ||
| ${INSTALL_PREFIX}/etcdctl version 2>/dev/null || ${INSTALL_PREFIX}/etcdctl --version | ||
| echo "ETCD_PATH=$(echo $INSTALL_PREFIX)" >> "$GITHUB_ENV" | ||
| echo "${INSTALL_PREFIX}" >> "$GITHUB_PATH" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add a newline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
driver/tkv/predicate.go
Outdated
| // FailedToEncodeTkvPredicateError is returned when we failed to encode tkvPredicate. | ||
| type FailedToEncodeTkvPredicateError struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same about the too long naming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
driver/tcs/integration_test.go
Outdated
| // skipIfNoTarantool skips the test if no Tarantool instance is available. | ||
| func skipIfNoTarantool(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need skip tests with TCS in case of Tarantool CE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Watch(ctx context.Context, key []byte, opts ...watch.Option) <-chan watch.Event | ||
| // The returned cleanup function should be called to stop the watch and release resources. | ||
| // An error is returned if the watch could not be established. | ||
| Watch(ctx context.Context, key []byte, opts ...watch.Option) (<-chan watch.Event, func(), error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a tests that the TCS driver implements the Driver interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used, instead:
Line 33 in 5285223
| _ driver.Driver = &Driver{} //nolint:exhaustruct |
driver/tkv/integration_test.go
Outdated
| @@ -0,0 +1,771 @@ | |||
| package tkv_test | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- There are a lack of negative tests. At least without a successful connect.
- Too complex nagative scenarios should be tested with unit-tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- added test for unsuccesfull connect (== no rw instance in pool
- 👍
| // New creates a new Tarantool driver instance. | ||
| // It establishes connections to Tarantool instances using the provided addresses. | ||
| func New(doer DoerWatcher) *Driver { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add two examples of usage the driver: Execute & Watcher.
The examples could be added into a root directory of the project to example_test.go (I prefer the way).
Or in the package.
Up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
5414399 to
bdd88e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
bdd88e7 to
3d42b92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
16fb273 to
1b7f28b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- New Tarantool Config Storage (TCS) driver package (driver/tcs). - Integration testing for TCS. - Updated .golangci.yml. - Modified core driver interfaces to be compatible with tcs. - Update go module dependencies. Closes #TNTP-4188
1b7f28b to
e2cc7fa
Compare
Closes #TNTP-4188